ci: use setup-dotnet built-in NuGet cache#5635
Conversation
Replaces the dedicated actions/cache step with setup-dotnet's built-in cache, keyed on Directory.Packages.props (sole source of versions under Central Package Management).
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Code Review
Clean simplification overall. The change is straightforward and the PR description is refreshingly honest about the trade-offs.
What's good
- Removing the hand-rolled
actions/cachestep in favour ofsetup-dotnet's built-in cache reduces moving parts and maintenance burden. - Keying only on
Directory.Packages.propsis sound reasoning for a Central Package Management repo — package versions are fully centralised there, so hashing every*.csprojwas redundant (and added key churn whenever any project file changed, even for non-package changes).
Potential concerns
1. Loss of continue-on-error
The removed step had continue-on-error: true, which meant a transient cache-service outage would silently fall through to a full restore without breaking the build. The setup-dotnet cache doesn't expose that option. If GitHub's cache service is temporarily unavailable the step could fail and block CI. In practice setup-dotnet probably handles this gracefully internally, but it's worth verifying against its source or confirming from a CI run that shows a cache miss scenario (not just a hit).
2. Relative path vs. glob for cache-dependency-path
The original key used **/Directory.Packages.props — a glob that would pick up any nested props files. The new cache-dependency-path: Directory.Packages.props is a relative path rooted at the repo root. If the project ever gains a sub-directory with its own Directory.Packages.props (e.g. a samples or docs sub-repo), those changes won't bust the cache. Low risk right now, but something to keep in mind.
3. Edge case: new package reference without a version bump
If a PackageReference is added to a *.csproj for a package whose version is already declared in Directory.Packages.props (e.g. a second project starts using a package the first already used), the cache key won't change but the restored cache will be stale until the next full miss. This is genuinely rare with CPM and the overhead is just one extra restore, so it's not a blocker — just good to document in a follow-up comment or ADR if the team cares about cache correctness guarantees.
Summary
The acknowledged restore-keys trade-off is acceptable — partial restores are nice-to-have, not critical. The change is a net improvement in simplicity. The continue-on-error point is the only one worth a quick sanity-check before merging; if the CI run on this PR completes cleanly (including a forced-miss scenario) it can be considered confirmed.
Summary
actions/cachestep for NuGet packages in.github/workflows/dotnet.ymlin favour ofactions/setup-dotnet's built-in cache.Directory.Packages.propsonly — with Central Package Management enabled, it's the sole source of package versions, so hashing csproj files adds nothing.Trade-off
setup-dotnet's cache does not exposerestore-keys, so a version bump now triggers a full cache miss instead of a partial restore. Simpler config in exchange for occasional full re-downloads.Test plan
Directory.Packages.propschange reports a cache hit